[SYCL] Allow SYCL_EXTERNAL to be applied to a function with raw pointers #861
Conversation
asavonic
force-pushed
the
private/asavonic/sycl-external-ptr
branch
from
96fe0f6 to
f73cc65
Compare
There was a problem hiding this comment.
General feedback: IMO, the warning is strange. I understand that we can't just wipe out the diagnostic because it's required by the current spec, but warning about nothing (in case of our implementation), and which will be emitted because we are going to use possibility allowed by this change seems strange for me.
And there are several cases where external functions with pointers can be useful
I prefer explicitly mention some kind of such use cases...
| : Warning<"%0 attribute applied to a " | ||
| "%select{function with a raw pointer return type" | ||
| "|function with a raw pointer parameter type}1" | ||
| " may prevent optimizations related to address spaces">, |
There was a problem hiding this comment.
I'm not sure that "may prevent optimizations related to address spaces" is useful information. How about telling the truth: it should not really matter for our implementation, but it's not following the current SYCL spec.
Other option is to guard this behavior under some command line option, see #806 .
There was a problem hiding this comment.
How about "SYCL 1.2.1 specification does not allow %0 attribute applied to a %select.."?
There was a problem hiding this comment.
Yeah, something like this... But to be honest, I'm not very good at wording.
SYCL 1.2.1 specification does not allow SYCL_EXTERNAL to be applied to a function with a pointer argument or with a pointer return value. This is required to limit address space inference work to a single translation unit. Given that we make inference after we link all translation units together, it should not really matter now. And there are several cases where external functions with pointers can be useful, so this patch tuns an error into a warning. Signed-off-by: Andrew Savonichev <andrew.savonichev@intel.com>
asavonic
force-pushed
the
private/asavonic/sycl-external-ptr
branch
from
f73cc65 to
cdc6008
Compare
Do we have plans to forbid it by default and allow it if some compiler option is passed only? |
Both the option and the warning work fine for my use case. I don't have an opinion on what should be the default. |
How about the driver option then? |
Sounds like this should be a part of #806. |
What do you mean by that? I agree with @Fznamznon: if extension requires compiler option, it should be added together with the feature. |
What feature do you refer to? This patch turns an error into a warning. Specification doesn't mention what exactly should happen, so it is an NFC patch from the specification perspective. |
…cl-strict Signed-off-by: Andrew Savonichev <andrew.savonichev@intel.com>
Signed-off-by: Andrew Savonichev <andrew.savonichev@intel.com>
Signed-off-by: Andrew Savonichev <andrew.savonichev@intel.com>
I disagree. I think the spec intention is clear that this should be an error. I think this needs an extension doc in sycl/sycl/doc/extensions and needs to be only enabled if the extension is enabled. |
The latest revision of the patch works as you described. |
SYCL 1.2.1 specification does not allow SYCL_EXTERNAL to be applied to
a function with a pointer argument or with a pointer return
value. This is required to limit address space inference work to a
single translation unit.
Given that we make inference after we link all translation units
together, it should not really matter now. And there are several cases
where external functions with pointers can be useful, so this patch
tuns an error into a warning.
Signed-off-by: Andrew Savonichev andrew.savonichev@intel.com